OCPNODE-3863: Add CRIOCredentialProviderConfig API #2557
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @QiWang19! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XL
QiWang19
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
QiWang19
force-pushed
the
creds-provider
branch
from
c00c122 to
1db4267
Compare
|
/retest-required |
QiWang19
force-pushed
the
creds-provider
branch
2 times, most recently
from
8805169 to
108219f
Compare
|
/test integration |
QiWang19
force-pushed
the
creds-provider
branch
from
108219f to
066bf58
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
PR has been reviewed by claude code /api-review. It helped run |
|
|
||
| // +kubebuilder:validation:MaxLength=512 | ||
| // +kubebuilder:validation:XValidation:rule=`self.matches('^((\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)(\\.(\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?))*)(:[0-9]+)?(/[-a-zA-Z0-9_/]*)?$')`,message="invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path" | ||
| type MatchImage string |
There was a problem hiding this comment.
@saschagrunert PTAL. as mentioned in the previsou enhancement discussion(openshift/enhancements#1861 (comment)), this is a stricter rule than the upstream matchImages, as it does not allow wildcard matching of partial subdomains like app*.k8s.io. Customers may raise concerns about this difference, but it simplifies the configuration.
What do you think?
There was a problem hiding this comment.
I mean, we can always make it looser later on while starting with a stricter regex. It's already fairly complex and we need extensive testing on that validation part.
The docs needs to be updated for this type, like:
// MatchImage is a string pattern used to match container image registry addresses.
// It must be a valid fully qualified domain name with optional wildcard, port, and path.
// The maximum length is 512 characters.
//
// Wildcards ('*') are supported for full subdomain labels and top-level domains.
// Each entry can optionally contain a port (e.g., :8080) and a path (e.g., /path).
// Wildcards are not allowed in the port or path portions.
//
// Examples:
// - "registry.io" - matches exactly registry.io
// - "*.azurecr.io" - matches any single subdomain of azurecr.io
// - "registry.io:8080/path" - matches with specific port and path prefix
//
// +kubebuilder:validation:MaxLength=512
// +kubebuilder:validation:XValidation:rule=`self.matches('^((\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)(\\.(\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?))*)(:[0-9]+)?(/[-a-zA-Z0-9_/]*)?$')`,message="invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path"
type MatchImage string
|
|
||
| // +kubebuilder:validation:MaxLength=512 | ||
| // +kubebuilder:validation:XValidation:rule=`self.matches('^((\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)(\\.(\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?))*)(:[0-9]+)?(/[-a-zA-Z0-9_/]*)?$')`,message="invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path" | ||
| type MatchImage string |
There was a problem hiding this comment.
I mean, we can always make it looser later on while starting with a stricter regex. It's already fairly complex and we need extensive testing on that validation part.
The docs needs to be updated for this type, like:
// MatchImage is a string pattern used to match container image registry addresses.
// It must be a valid fully qualified domain name with optional wildcard, port, and path.
// The maximum length is 512 characters.
//
// Wildcards ('*') are supported for full subdomain labels and top-level domains.
// Each entry can optionally contain a port (e.g., :8080) and a path (e.g., /path).
// Wildcards are not allowed in the port or path portions.
//
// Examples:
// - "registry.io" - matches exactly registry.io
// - "*.azurecr.io" - matches any single subdomain of azurecr.io
// - "registry.io:8080/path" - matches with specific port and path prefix
//
// +kubebuilder:validation:MaxLength=512
// +kubebuilder:validation:XValidation:rule=`self.matches('^((\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)(\\.(\\*|[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?))*)(:[0-9]+)?(/[-a-zA-Z0-9_/]*)?$')`,message="invalid matchImages value, must be a valid fully qualified domain name with optional wildcard, port, and path"
type MatchImage string| // status represents the current state of the CRIOCredentialProviderConfig. | ||
| // +optional | ||
| Status *CRIOCredentialProviderConfigStatus `json:"status,omitempty"` |
There was a problem hiding this comment.
Optional fields must explain what happens when they are omitted.
| // status represents the current state of the CRIOCredentialProviderConfig. | |
| // +optional | |
| Status *CRIOCredentialProviderConfigStatus `json:"status,omitempty"` | |
| // status represents the current state of the CRIOCredentialProviderConfig. | |
| // When omitted or nil, it indicates that the status has not yet been set by the controller. | |
| // The controller will populate this field with validation conditions and operational state. | |
| // +optional | |
| Status *CRIOCredentialProviderConfigStatus `json:"status,omitempty"` |
| // conditions represent the latest available observations of the configuration state | ||
| // +optional | ||
| // +kubebuilder:validation:MaxItems=4 | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` |
There was a problem hiding this comment.
This field has three validation markers (MaxItems, listType, listMapKey) that must all be documented. The comment must explain:
- behavior when omitted
- the maximum of 4 items
- that it's a map type keyed by type
- what condition types are expected based on the constants defined in the file
| // conditions represent the latest available observations of the configuration state | |
| // +optional | |
| // +kubebuilder:validation:MaxItems=4 | |
| // +listType=map | |
| // +listMapKey=type | |
| Conditions []metav1.Condition `json:"conditions,omitempty"` | |
| // conditions represent the latest available observations of the configuration state. | |
| // When omitted or empty, it indicates that no conditions have been reported yet. | |
| // The maximum number of conditions is 4. | |
| // Conditions are stored as a map keyed by condition type, ensuring uniqueness. | |
| // | |
| // Expected condition types include: | |
| // - "Validated": indicates whether the matchImages configuration is valid | |
| // +optional | |
| // +kubebuilder:validation:MaxItems=4 | |
| // +listType=map | |
| // +listMapKey=type | |
| Conditions []metav1.Condition `json:"conditions,omitempty"` |
| // spec defines the desired configuration of the CRIO Credential Provider. | ||
| // +required | ||
| Spec CRIOCredentialProviderConfigSpec `json:"spec,omitzero"` |
There was a problem hiding this comment.
Per API conventions, required fields should explicitly state in their documentation that they are required.
| // spec defines the desired configuration of the CRIO Credential Provider. | |
| // +required | |
| Spec CRIOCredentialProviderConfigSpec `json:"spec,omitzero"` | |
| // spec defines the desired configuration of the CRIO Credential Provider. | |
| // This field is required and must be provided when creating the resource. | |
| // +required | |
| Spec CRIOCredentialProviderConfigSpec `json:"spec,omitzero"` |
| // passed to the kubelet CredentialProviderConfig, and if any pattern matches | ||
| // the requested image, CRI-O credential provider will be invoked to obtain credentials for pulling | ||
| // that image or its mirrors. | ||
| // |
There was a problem hiding this comment.
We're missing the docs for min and max items here, also that it refers to a set:
| // | |
| // | |
| // This field is required and must contain between 1 and 50 entries. | |
| // The list is treated as a set, so duplicate entries are not allowed. | |
| // |
| const ( | ||
| // ConditionTypeValidated indicates whether the configuration is failed, or partially valid | ||
| ConditionTypeValidated = "Validated" | ||
|
|
||
| // ReasonValidationFailed indicates the MatchImages configuration contains invalid patterns | ||
| ReasonValidationFailed = "ValidationFailed" | ||
|
|
||
| // ReasonConfigurationPartiallyApplied indicates some matchImage entries were ignored due to conflicts | ||
| ReasonConfigurationPartiallyApplied = "ConfigurationPartiallyApplied" | ||
| ) |
There was a problem hiding this comment.
We should add comprehensive documentation about their meaning and usage:
| const ( | |
| // ConditionTypeValidated indicates whether the configuration is failed, or partially valid | |
| ConditionTypeValidated = "Validated" | |
| // ReasonValidationFailed indicates the MatchImages configuration contains invalid patterns | |
| ReasonValidationFailed = "ValidationFailed" | |
| // ReasonConfigurationPartiallyApplied indicates some matchImage entries were ignored due to conflicts | |
| ReasonConfigurationPartiallyApplied = "ConfigurationPartiallyApplied" | |
| ) | |
| const ( | |
| // ConditionTypeValidated is a condition type that indicates whether the CRIOCredentialProviderConfig | |
| // matchImages configuration has been validated successfully. | |
| // When True, all matchImage patterns are valid and have been applied. | |
| // When False, the configuration contains errors (see Reason for details). | |
| // Possible reasons for False status: | |
| // - ValidationFailed: matchImages contains invalid patterns | |
| // - ConfigurationPartiallyApplied: some matchImage entries were ignored due to conflicts | |
| ConditionTypeValidated = "Validated" | |
| // ReasonValidationFailed is a condition reason used with ConditionTypeValidated=False | |
| // to indicate that the matchImages configuration contains one or more invalid registry patterns | |
| // that do not conform to the required format (valid FQDN with optional wildcard, port, and path). | |
| ReasonValidationFailed = "ValidationFailed" | |
| // ReasonConfigurationPartiallyApplied is a condition reason used with ConditionTypeValidated=False | |
| // to indicate that some matchImage entries were ignored due to conflicts or overlapping patterns. | |
| // The condition message will contain details about which entries were ignored and why. | |
| ReasonConfigurationPartiallyApplied = "ConfigurationPartiallyApplied" | |
| ) |
QiWang19
force-pushed
the
creds-provider
branch
from
066bf58 to
d2a191b
Compare
- Add feature gate for CRIOCredentialProviderConfig in various feature gate manifests. Signed-off-by: Qi Wang <[email protected]>
QiWang19
force-pushed
the
creds-provider
branch
from
d2a191b to
41bd600
Compare
openshift-ci
bot
added
size/XXL
QiWang19
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@QiWang19: This pull request references OCPNODE-3863 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@QiWang19: This pull request references OCPNODE-3863 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@QiWang19: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| } | ||
|
|
||
| const ( | ||
|
|
3 participants
enhancement: OCPNODE-3753: CRIOCredentialProviderConfig for Namespace-Scoped Mirror Authentication enhancements#1861
Add CRIOCredentialProviderConfig API setting configs https://github.com/cri-o/crio-credential-provider for retrieving mirror pull secrets
Add feature gate for CRIOCredentialProviderConfig feature gate.